Sheffield | 25-SDC-Nov | Sheida Shabankari | Sprint 3 |Implement shell tools#241
Sheffield | 25-SDC-Nov | Sheida Shabankari | Sprint 3 |Implement shell tools#241sheida-shab wants to merge 17 commits intoCodeYourFuture:mainfrom
Conversation
LonMcGregor
left a comment
There was a problem hiding this comment.
Good work on these tasks, apologies for the lateness in review
You have taken a good approach at manually parsing the arguments. You might consider investigating if there are any libraries or APIs you could use to help you with this. In a more complex shell tool, it might be difficult to manually check arguments this way. It would also make it easier to combine arguments, e.g.allowing wc -w -l instead of only one at a time
implement-shell-tools/cat/cat.js
Outdated
| lineNumber ++ ; | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
The formatting is a bit unclear here, do you know of any tools in the command line or the IDE that could help to improve this?
There was a problem hiding this comment.
I’m using VS Code with Prettier enabled, but I’m not sure why the file wasn’t properly formatted when saved. I manually reformatted it afterwards.
implement-shell-tools/cat/cat.js
Outdated
| const filesNameArray = await glob(fileNamePattern); | ||
| filesNameArray.sort(); | ||
| let lineNumber = 1; | ||
| //console.log(filesNameArray); |
There was a problem hiding this comment.
Tip: Avoid committing commented old code
| @@ -0,0 +1,46 @@ | |||
| import { promises as fs } from "node:fs"; | |||
| import { glob } from "glob"; | |||
There was a problem hiding this comment.
When I run this code I get an error Cannot find package 'glob'. Do you see this also?
There was a problem hiding this comment.
Thanks for your comment but I don't see any error !
implement-shell-tools/ls/ls.js
Outdated
| import { promises as fs } from "node:fs"; | ||
| async function listDir() { | ||
| let finalFileList; | ||
| const flag = process.argv[2]; |
There was a problem hiding this comment.
The readme also asks you to try implementing the -1 option, can you try this?
There was a problem hiding this comment.
When I initially implemented the code, the -1 flag behavior was applied automatically by default, so I didn’t explicitly implement it. However, based on your feedback, I have now updated the implementation to handle the -1 flag explicitly.
implement-shell-tools/wc/wc.js
Outdated
|
|
||
| for(const file of commandLineArray){ | ||
| const fileContent=await fs.readFile(file , 'utf-8'); | ||
| const fileBuffer=await fs.readFile(file); |
There was a problem hiding this comment.
Do you need to read the file in two times? What impact will this have on performance?
There was a problem hiding this comment.
You're right — the file is being read twice unnecessarily, which affects performance. This could be improved by reading the file once and reusing the buffer. I updated the code.
implement-shell-tools/wc/wc.js
Outdated
| if(commandLineArray.length>1){ | ||
| switch (flags[0]) { | ||
| case "-l": | ||
| console.log(`${String(totalLines).padStart(4)} total`); |
There was a problem hiding this comment.
There is some duplicated code here, can you reduce that?
There was a problem hiding this comment.
I fixed it by implementing a function.
|
Good work on this. |
9138dac to
5b9cc2d
Compare
|
Thanks for your feedback.I think I fixed the issue. |
|
This looks good, good work |
Self checklist